SOLR-17316: make SolrJ response objects work with non-binary ResponseParsers - #4640
SOLR-17316: make SolrJ response objects work with non-binary ResponseParsers#4640serhiy-bzhezytskyy wants to merge 13 commits into
Conversation
…arsers getStatus() and getQTime() cast the header value to Integer, which threw a ClassCastException under a parser that yields a different numeric type (the JSON parser yields Long). Widen via Number.intValue() instead.
The SolrJ response classes assume the Java types the binary parser produces, so
reading a response parsed by a non-binary parser (e.g. the JSON map parser) threw
ClassCastException: JSON yields raw Map/List where the code expects
NamedList/SolrDocumentList, and Long where it casts to Integer.
- ResponseNormalizer converts a parsed response into the canonical shape (nested
objects -> NamedList/SimpleOrderedMap, a {numFound,docs} object ->
SolrDocumentList); it is a no-op for already-canonical binary/XML responses.
- ResponseParser.producesCanonicalForm() gates it; JsonMapResponseParser returns
false. HttpSolrClient normalizes at the shared response boundary, covering both
the JDK and Jetty transports while binary/XML pay nothing.
- Remaining numeric reads that cast to Integer/Float are widened via Number
(grouping, interval and pivot facet counts, spellcheck, analysis token offsets,
Luke, schema version).
Tests cover the normalizer, cross-format parity (binary/XML/json-map), each
affected section, and an end-to-end HTTP query with the JSON parser on both
transports.
|
Are there any objections to its merger? I am ready to resolve them, just let me know. Thanks |
…e/style fixes Review feedback on apache#4640. The client no longer knows which parsers need normalizing. ResponseParser gains processCanonicalResponse(), which defaults to processResponse() — most parsers are canonical already and inherit it unchanged — and JsonMapResponseParser overrides it to convert. The producesCanonicalForm() predicate is gone, and HttpSolrClient just calls the one method: rsp = processor.processCanonicalResponse(is, encoding); The existing processResponse() is untouched, since 20 call sites use it directly, including the error path in ConcurrentUpdateBaseSolrClient which only reads resp.get("error") and needs no conversion. ResponseNormalizer moves from org.apache.solr.common.util to org.apache.solr.client.solrj.response: it is not a common utility. A plain NamedList is no longer promoted to SimpleOrderedMap. SimpleOrderedMap implements Map and is written differently by the response writers -- a JSON writer renders it as {"foo":10} and a NamedList as ["foo",10] -- so widening the type changes the contract of the value. Only the concrete type is preserved now; a JSON object still becomes a SimpleOrderedMap, since its keys are unique by construction. Worth noting the mutation check: with "always promote" in place, all 27 existing tests passed, so two tests were added for the distinction and they do fail on it. Also: pattern-matching instanceof throughout ResponseNormalizer, text blocks for the JSON literals in the parity test, no FQNs, and no try-with-resources around solrTestRule.getSolrClient() -- its javadoc says "The caller doesn't need to close it". ResponseParserCanonicalFormTest becomes ResponseParserCanonicalResponseTest and pins behaviour rather than the removed predicate: the JSON parser's raw output is Maps, its canonical output is NamedLists and a SolrDocumentList, and a canonical parser passes through unchanged. 200 solrj response/client tests pass; :solr:solrj:check clean.
… map parser Extends the randomization SOLR-15070 introduced in that test — javabin or xml — to pick among three parsers, so the JSON map parser goes through the same suggester assertions as the other two. It is a regression test for this PR rather than added coverage: on the commit before the normalizer, forcing that parser in fails all three of the test's methods with ClassCastException: class java.util.LinkedHashMap cannot be cast to class org.apache.solr.common.util.NamedList and with the conversion in place they pass. Mutation-checked — removing the conversion from JsonMapResponseParser#processCanonicalResponse brings the ClassCastException back under -Ptests.iters=10.
The test fed the JSON parser a literal whose facet_fields was written in json.nl=map form, which JsonMapResponseParser never requests, so the input shape does not occur on a live request — running the response classes end-to-end over a real server surfaced that same section failing as an array. Two of its three methods also asserted existing javabin and xml behaviour rather than anything this change introduces. Coverage of the numeric widening in QueryResponse is kept by QueryResponseSectionParityTest, and of a real request by QueryResponseJsonParserIntegrationTest.
…and have the JSON map parser ask for json.nl=map JsonMapResponseParser could not read the response it was getting. Under the default json.nl=flat a NamedList is written as an array of alternating names and values, so facet_fields arrived as a List where the response classes expect a NamedList, and the structure cannot be recovered after the fact. The style had to be set by hand on every request, which is not something a caller should have to know per parser. ResponseParser#getRequestParams supplies params alongside wt. Anything the request set explicitly wins, so this only provides defaults. Applied in HttpSolrClient#initializeSolrParams, which every HTTP client routes through. QueryResponseJsonParserIntegrationTest no longer sets json.nl itself, which is what the change is for, and asserts that an explicit value survives.
JSON has no document type, so the JSON writer emits nested documents as a _childDocuments_ field holding a list of maps. The binary and XML parsers hand them back as child documents; this one left them as a plain field, so SolrDocument#hasChildDocuments was false and the children were unreachable through the documented accessors. Nesting is recursive, so grandchildren are covered too.
facet_queries, a range facet's counts and a pivot's query counts were cast to NamedList<Integer> and iterated as Integer entries, so a response whose numbers arrive as Long threw ClassCastException. Same defect as the accessors already widened here, in three places the earlier commits did not reach; the values are still narrowed to int, so nothing about the public types changes.
SolrExampleTests has a subclass per parser and JSON was missing, so the whole 42-test suite now runs against JsonMapResponseParser. That is what found the three defects fixed in the preceding commits; on the commit before them it fails seven ways. Nine assertions in SolrExampleTests pinned the boxed type of a number rather than its value -- (Integer) getFieldValue(..), assertEquals(1.0f, ..), RangeFacet<Float, Float> -- and are relaxed to Number where the value is the point. Nothing is ignored: 42 of 42 pass, and the binary, XML, CBOR and HTTP/2 subclasses are unaffected.
There was a problem hiding this comment.
I see you are supporting anonymous child documents -- which is the original thing and I've been meaning to deprecate it. Nowadays, we do "nested documents", which have named relationships from parent to child. Neither is reflected in the schema, but anyway you can fetch children (named or anonymous) via fl=*,[child fl=*] if I recall off the top of my head.
There was a problem hiding this comment.
thanks for this. It's good.
However, note that most of Lucene & Solr's randomized testing is done at a deeper level such that an individual test generally doesn't even have to do anything to get the randomization -- it just happens at a deeper test framework/infra level. For example... imagine if the default was a settable static supplier... and imagine if SolrTestCase were to set it. Then the useful test coverage would go through the roof (thousands of Solr tests) and we'd probably toss aside more of your tests as redundant. I'm hesitant to truly recommend precisely this... but I'm at least taking the educational opportunity of sharing the rather unique randomized testing philosophy that permeates the Lucene & Solr projects.
Uses SolrParams.wrapDefaults and SolrParams.of instead of hand-rolled merging, and renames getRequestParams to getAdditionalRequestParams. Parser-required params now take precedence over the request's own, as wt already did — a parser that can't read the form the caller asked for would fail rather than honour it. EmbeddedSolrServer called processResponse directly, so a JSON parser there still threw ClassCastException. It now applies the parser's params and reads the response canonically. Named nested documents are reconstructed too, keyed on _nest_path_.
The only textual conflict was two imports in HttpSolrClient: SOLR-18351 removed ContentStream from this file, SOLR-17316 added SolrParams. Compilation then failed on ResponseNormalizerTest, because SOLR-18353 removed SolrDocument.getChildDocumentCount() -- switched to getChildDocuments().size().
dsmiley
left a comment
There was a problem hiding this comment.
Great work here @serhiy-bzhezytskyy ; I'm planning on merging Monday. It's ready now, honestly, just want to give @gerlowskija a little more time to look and to debate the changelog type.
| SolrJ's QueryResponse and other response objects now work when the client is configured with a | ||
| non-binary response parser (such as the JSON parser); previously their accessors could throw a | ||
| ClassCastException. | ||
| type: fixed |
There was a problem hiding this comment.
| type: fixed | |
| type: added |
| non-binary response parser (such as the JSON parser); previously their accessors could throw a | ||
| ClassCastException. |
There was a problem hiding this comment.
| non-binary response parser (such as the JSON parser); previously their accessors could throw a | |
| ClassCastException. | |
| JSON response parser (namely `JsonMapResponseParser`); | |
| previously their accessors could throw ClassCastException. |
I think we should just name JsonMapResponseParser specifically, as we know this works. JacksonDataBindResponseParser probably doesn't work. Albeit it'd be interesting to try experimentally. I'm definitely keen on use of Jackson for CBOR support. For Solr 11, I hope we can use Jackson inside JsonMapResponseParser instead of noggit, but that's a separate discussion.
Nothing changes for callers on the default binary response parser; the benefit requires configuring a non-binary one, which is the opt-in criterion dev-docs/changelog.adoc gives for "added". Retitled so the entry states the new capability instead of opening on a ClassCastException, which read wrong under an "Added" heading.
https://issues.apache.org/jira/browse/SOLR-17316
SolrJ's response classes assume binary-parser types, so reading a response parsed by a non-binary parser (the JSON parser) throws ClassCastException. Two commits so the small safe fix can go in on its own if you'd rather.
Commit 1 is the narrow fix: getStatus/getQTime cast to Integer, which CCEs when JSON gives Long. Widened via Number. That's it — resolves the getters the issue names.
Commit 2 goes further. It's not just the header — getResults(), facets, grouping all break under JSON too, because the parser hands back raw Map/List where the code wants NamedList/SolrDocumentList. So there's a ResponseNormalizer that converts a parsed response to the canonical shape at the client boundary (no-op for binary/XML), gated by a new ResponseParser.producesCanonicalForm() so only the JSON map parser triggers it. Both transports covered, binary pays nothing. Plus the remaining Integer/Float casts widened.
Heads up: commit 2 basically does what SOLR-3451 asked for in 2012, which was closed Won't Fix ("solr does not have a way to write a JSON response and read the same value"). Still true for json.nl=flat since it's lossy, but json.nl=map round-trips and the normalizer does the rest. So I'd treat commit 2 as reopening that discussion — fine to split it out or take it to dev@ if you'd prefer, commit 1 stands alone either way.
Tests: the normalizer + edge cases, binary/xml/json-map parity, the affected sections (grouping, facets, stats, spellcheck, highlighting, terms, moreLikeThis, analysis, Luke, schema), and an end-to-end JSON query on both the Jetty and JDK clients.